Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix!: always prompt user about misconfigured inputs #309

Merged
merged 2 commits into from
Apr 25, 2024
Merged

fix!: always prompt user about misconfigured inputs #309

merged 2 commits into from
Apr 25, 2024

Conversation

marofke
Copy link
Contributor

@marofke marofke commented Apr 22, 2024 •
edited
Loading

What was the problem/requirement? (What/Why)

We need to better handle some job attachments paths (inputs, outputs, references), specifically around empty/non-existent and misconfigured (i.e. files marked as directories and vice-versa)

What was the solution? (How)

Validate paths better. If any inputs are specified that do not exist, we add them to referenced paths, or fail submission if the new option require_paths_exist is used (either through the CLI flag --require-paths-exist or a checkbox in the Job Attachments tab). Empty input directories are likewise added as referenced paths. Throw an error for any paths that were marked as files but are actually directories.

What is the impact of this change?

Users submitting jobs can be confident all of their expected inputs have been uploaded/included, and have more insight into what is actually being included in their submissions.

How was this change tested?

hatch run fmt
hatch run lint
hatch build
hatch run test

Setup

  • I crafted two different kinds of job bundles which I used for various job submissions
    1. Defined IN, INOUT, and OUT parameters, added asset_references and parameter_values that referenced files that did and did not exist, and some directories that were empty
    2. Defined OUT parameters, and added asset_references for input directories that were empty and some that did not exist

Test 1: Submitting Within A Configured Storage Profile Location

  1. (GUI) Successful Submission With Confirmation Popup

Test1

  1. (GUI) Unsuccessful Submission when setting 'Require Paths Exist' checkbox

Test2

  1. (CLI) Successful Submission With Confirmation Prompt
marofke@ Desktop % deadline bundle submit test_bundle_1    
Submitting to Queue: CMF
INFO:deadline.client.api._submit_job_bundle:Input path './other_input_dir' does not exist. Adding to referenced paths.
INFO:deadline.client.api._submit_job_bundle:Input path '/Users/marofke/Desktop/asset_ref_dir/does_not_exist' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path './other.txt' resolving to '/Users/marofke/Desktop/other.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/test.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/test.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/test copy.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/test copy.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/does not exist.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/does not exist.txt' does not exist. Adding to referenced paths.
Job submission contains 18 input files totaling 20.8 KB.  All input files will be uploaded to S3 if they are not already present in the job attachments bucket.

Do you wish to proceed? [Y/n]:
  1. (CLI) Unsuccessful Submission when using --require-paths-exist flag
marofke@ Desktop % deadline bundle submit test_bundle_1 --yes --require-paths-exist
Submitting to Queue: CMF
Job submission contains misconfigured input directories and cannot be submitted. All input directories must exist.
Non-existent directories:
	./other_input_dir
	/Users/marofke/Desktop/asset_ref_dir/does_not_exist
Job submission canceled.
  1. (GUI and CLI) Successfully Submitted my second job bundle that only had output directories

Test 2: Submitting With No Configured Storage Profile Location

  1. (GUI) Successful Submission With Confirmation Popup
  • confirmed setting the auto-accept config setting still showed this prompt

Test3

  1. (CLI) Successful Submission With Confirmation Popup
  • confirmed setting the auto-accept config setting still showed this prompt
marofke@ Desktop % deadline bundle submit test_bundle_1
Submitting to Queue: CMF
INFO:deadline.client.api._submit_job_bundle:Input path './other_input_dir' does not exist. Adding to referenced paths.
INFO:deadline.client.api._submit_job_bundle:Input path '/Users/marofke/Desktop/asset_ref_dir/does_not_exist' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/test.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/test.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path './other.txt' resolving to '/Users/marofke/Desktop/other.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/does not exist.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/does not exist.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path 'Users/marofke/Desktop/asset_ref_dir/test copy.txt' resolving to '/Users/marofke/Desktop/Users/marofke/Desktop/asset_ref_dir/test copy.txt' does not exist. Adding to referenced paths.
Job submission contains 18 input files totaling 20.8 KB.  All input files will be uploaded to S3 if they are not already present in the job attachments bucket.

Files were specified outside of the configured storage profile location(s).  Please confirm that you intend to submit a job that uses files from the following directories:

Under the directory '/Users/marofke/Desktop':
	18 input files
	2 output directories
	6 referenced files and/or directories

To permanently remove this warning you must only use files located within a storage profile location.

Do you wish to proceed? [y/N]:
  1. (GUI and CLI) Submitted my second job bundle, confirming it also prompted when output paths fell outside of a storage profile filesystem location

Test 3: Submitting Job with Directory Marked as File

  1. (GUI) Confirmed it failed to submit

Test16

  1. (CLI) Confirmed it failed to submit
marofke@ Desktop % deadline bundle submit test_bundle_1    
Submitting to Queue: CMF
INFO:deadline.client.api._submit_job_bundle:Input path './other_input_dir' does not exist. Adding to referenced paths.
INFO:deadline.client.api._submit_job_bundle:Input path '/Users/marofke/Desktop/asset_ref_dir/does_not_exist' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path './other.txt' resolving to '/Users/marofke/Desktop/other.txt' does not exist. Adding to referenced paths.
INFO:deadline.job_attachments.upload:Input path '/Users/marofke/Desktop/asset_ref_dir/does not exist.txt' resolving to '/Users/marofke/Desktop/asset_ref_dir/does not exist.txt' does not exist. Adding to referenced paths.
Job submission contains missing input files or directories specified as files. All inputs must exist and be classified properly.
Directories classified as files:
	/Users/marofke/Desktop/asset_ref_dir/nested
Job submission canceled.

UI Changes

  • Here's the new UI setting that allows users to enforce all paths exist:

    Show / Hide

    NewCheckbox

Was this change documented?

No

Is this a breaking change?

Yes, added new CLI flag and reworked the public interface of the Job Attachments S3AssetUploader

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@marofke marofke force-pushed the marofke/update_upload_warnings branch 8 times, most recently from ef3c9e9 to ee0f962 Compare April 23, 2024 21:39
@marofke marofke force-pushed the marofke/update_upload_warnings branch 2 times, most recently from 491a523 to d391e12 Compare April 25, 2024 00:13
@marofke marofke marked this pull request as ready for review April 25, 2024 00:21
@marofke marofke requested a review from a team as a code owner April 25, 2024 00:21
chocecil
chocecil previously approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@chocecil chocecil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this path checking enabled by default? It appears to be quite useful, and it would be beneficial if we could always validate it.

@marofke
Copy link
Contributor Author

marofke commented Apr 25, 2024

Why isn't this path checking enabled by default? It appears to be quite useful, and it would be beneficial if we could always validate it.

Mark and I talked about this a bit before I made the change, and while we agreed enabling by default would be ideal, doing so would be a break in existing behaviour (I.e. isn't backwards compatible), as we currently only log if files are missing.

@chocecil
Copy link
Contributor

Why isn't this path checking enabled by default? It appears to be quite useful, and it would be beneficial if we could always validate it.

Mark and I talked about this a bit before I made the change, and while we agreed enabling by default would be ideal, doing so would be a break in existing behaviour (I.e. isn't backwards compatible), as we currently only log if files are missing.

Make sense.
I was thinking just printing warning messages but proceeding with submission isn't a non backwards change even when the flag is not given. The example in 3. (CLI) Successful Submission With Confirmation Prompt in the description seems to do that exactly.

@marofke marofke changed the title fix: always prompt user about misconfigured inputs fix!: always prompt user about misconfigured inputs Apr 25, 2024
@epmog @marofke
fix!: always prompt user about misconfigured inputs
89fbce2 
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
@marofke marofke force-pushed the marofke/update_upload_warnings branch from d391e12 to f2bd88d Compare April 25, 2024 17:19
@marofke
fix!: Add empty paths to referenced paths
d23abc0 
Signed-off-by: Caden Marofke <132690522+marofke@users.noreply.github.com>
@marofke marofke force-pushed the marofke/update_upload_warnings branch from f2bd88d to d23abc0 Compare April 25, 2024 18:00
@marofke marofke merged commit f8d5826 into aws-deadline:mainline Apr 25, 2024
15 checks passed
@marofke marofke deleted the marofke/update_upload_warnings branch April 25, 2024 18:44
@client-software-ci client-software-ci mentioned this pull request Apr 25, 2024
Merged
gahyusuh added a commit to gahyusuh/deadline-cloud that referenced this pull request Apr 26, 2024
@gahyusuh
chore: revise the squashed commit message for aws-deadline#309 in Cha…
3319af8 
…ngelog

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
gahyusuh added a commit to gahyusuh/deadline-cloud that referenced this pull request Apr 26, 2024
@gahyusuh
chore: revise the squashed commit message for aws-deadline#309 in Cha…
2cf4053 
…ngelog

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
@gahyusuh gahyusuh mentioned this pull request Apr 26, 2024
Merged
epmog pushed a commit to gahyusuh/deadline-cloud that referenced this pull request May 15, 2024
@gahyusuh @epmog
chore: revise the squashed commit message for aws-deadline#309 in Cha…
9f2283c 
…ngelog

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
epmog pushed a commit that referenced this pull request May 15, 2024
@gahyusuh
chore: revise the squashed commit message for #309 in Changelog (#318)
aa92985 
Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chocecil chocecil chocecil approved these changes

@mwiebe mwiebe mwiebe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marofke @chocecil @mwiebe @epmog